Add support for the simple "sigs-based auth" VSS scheme #755
Conversation
TheBlueMatt
commented
Jan 15, 2026
TheBlueMatt
commented
At lightningdevkit/vss-server#79 we added a new, trivial, VSS authentication scheme that ensures client isolation without much else. This is great for testing, and we expect some to do new-account-rate-limiting via other means, so might well become a common default. Here we add support to it in ldk-node.
When we added the trivial sigs-based authentication scheme in VSS, we made it the default if no other authentication scheme was configured and default features are enabled. This broke our integration tests as we were expecting no authentication to be required in such a case. Here we fix this by switching to the new sigs-based auth scheme, removing `store_id`s to demonstrate client isolation while we're at it.
|
👋 Thanks for assigning @tankyleo as a reviewer! |
|
Depends on lightningdevkit/vss-client#54 |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Generally looks good, some comments. Mind adding an intermittent commit that bumps the dependency to your branch of vss-client so we can see if CI passes?
| .build_with_vss_store( | ||
| config_a.node_entropy, | ||
| vss_base_url.clone(), | ||
| "node_1_store".to_string(), |
There was a problem hiding this comment.
Why are we changing the store_id here and below?
There was a problem hiding this comment.
To demonstrate/test client isolation in VSS by default.
There was a problem hiding this comment.
Hmm, not sure it's worth the changes, but if you think it somehow preferable, sure..
There was a problem hiding this comment.
Entirely up to you. I did it cause coverage is coverage, even if the coverage is in totally the wrong repo. If you don't want coverage in the wrong repo, that's also totally fine :).
There was a problem hiding this comment.
Yeah, feel free to leave as-is.
|
|
||
| let rand_suffix: String = | ||
| (0..7).map(|_| rng().sample(rand::distr::Alphanumeric) as char).collect(); | ||
| let store_id = format!("v0_compat_test_{}", rand_suffix); |
There was a problem hiding this comment.
No, please leave this in place, otherwise running this test repeatedly against the same backend won't start from scratch every time.
There was a problem hiding this comment.
Oops, sorry, missed that they're using a static seed. I randomized the seed.
There's not really any reason to force devs to select a `store_id` in the default `build_with_vss_store` anymore - we use a key derived from the node entropy to key the storage on the service side which should prevent anything else from accessing the same data unless its very deliberate. Thus, we simply drop the `store_id` parameter to `build_with_vss_store`.
|
Oops, fixed an issue but also pushed a new commit to drop the |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Generally looks good, would still be good to see a vss-client dependency bump (preferably before the next vss-client release so we get a chance to amend bugs).
I don't mind the test changes, but leaning NACK on the fixating the store_id suddenly, see below.
| .build_with_vss_store( | ||
| config_a.node_entropy, | ||
| vss_base_url.clone(), | ||
| "node_1_store".to_string(), |
There was a problem hiding this comment.
Hmm, not sure it's worth the changes, but if you think it somehow preferable, sure..
| &self, node_entropy: NodeEntropy, vss_url: String, fixed_headers: HashMap<String, String>, | ||
| ) -> Result<Node, BuildError> { | ||
| let logger = setup_logger(&self.log_writer_config, &self.config)?; | ||
| let store_id = "ldk-node".to_owned(); |
There was a problem hiding this comment.
I don't think we should do this. Adding pubkey-auth is just another authentication mechanism, which is otherwise unrelated to the VSS API contract. Note that we had users pick store_ids freely already and they are running in production. Suddenly fixating the store_id breaks the VSS API contract but also disallows them to switch to pubkey-auth from whatever they are running right now.
There was a problem hiding this comment.
I'm not quite sure I understand the argument around the API contract - there's an API contract defined in vss-client and between the VSS client and server, but I'm not really sure that applies to new instances of ldk-node using VSS. Do we really think they're going to re-derive the VSS auth key from their entropy in order to instantiate another instance of the VSS client with the same auth to store other stuff there and need to put it in the same store_id? If not, making them pass another argument just seems like yet more cognitive burden on devs to figure out what a store_id is.
disallows them to switch to pubkey-auth from whatever they are running right now.
Fwiw they can manually instantiate the vss-client VssHeaderProvider instance and call build_with_vss_store_and_header_provider, though we could also add a separate method if we anticipate users switching authentication mechanisms but keeping their existing VSS instance. It wasn't clear that we anticipate that kind of migration.
There was a problem hiding this comment.
I'm not quite sure I understand the argument around the API contract - there's an API contract defined in vss-client and between the VSS client and server, but I'm not really sure that applies to new instances of ldk-node using VSS.
Well, but we're re-exposing that API in LDK Node. In particular, we recently introduced VssStoreBuilder explicitly so that other projects (such as Orange) can build a VssStore instance outside of/prior to LDK Node initialization to be able to store some extra data. We also considered that a pre-factor to then finally upstream VssStore and VssStoreBuilder to lightning-persister soon, which has been the goal since the beginning, but we never go around to do it so far. That's all to say that I'm not quite seeing the need to fixate the store_id at all, and in fact we'll need to revert that (and then re-apply for backwards compat if we'd go that way) once we upstream the code.
Fwiw they can manually instantiate the vss-client VssHeaderProvider instance and call build_with_vss_store_and_header_provider, though we could also add a separate method if we anticipate users switching authentication mechanisms but keeping their existing VSS instance. It wasn't clear that we anticipate that kind of migration.
I mean, we should anticipate such a migration, as we have a bunch of users on JWT right now, that might opt to choose the new auth mechanism which seems to be the new default over time? At least I don't think we should make it impossible or have them some custom route just to do that? FWIW, we also have MigratableKVStore, why not allow users to build two independent VssStores with different store_id (but both using same auth) and then migrate between them, e.g., for backup purposes.
There was a problem hiding this comment.
Now opened a draft for this lightningdevkit/rust-lightning#4323 (i.e., draft until this lands so we upstream the latest version).
There was a problem hiding this comment.
Well, but we're re-exposing that API in LDK Node. In particular, we recently introduced VssStoreBuilder explicitly so that other projects (such as Orange) can build a VssStore instance outside of/prior to LDK Node initialization to be able to store some extra data. We also considered that a pre-factor to then finally upstream VssStore and VssStoreBuilder to lightning-persister soon, which has been the goal since the beginning, but we never go around to do it so far. That's all to say that I'm not quite seeing the need to fixate the store_id at all, and in fact we'll need to revert that (and then re-apply for backwards compat if we'd go that way) once we upstream the code.
Right, the change here did not remove the store-id parameter from the VssStoreBuilder, but only from the NodeBuilder::build_with_vss_store method that uses an underlying VssStoreBuilder. My assumption here was that we wanted as simple an API as possible under the new sigs-based auth scheme, and for those using NodeBuilder::build_with_vss_store we assume that they are just using LDK Node, as they would otherwise use a VssStoreBuilder and pass a reference to the VSS Store to NodeBuilder::build_with_store so that they can also reuse the VSS Store elsewhere in their code.
I guess a related question here is whether something like orange can/should/will use a separate store-id from ldk node. I assume that it should, given we don't want it storing stuff somewhere LDK Node might want to in the future? In that case it would need multiple VSS Store objects. Sadly I believe the store-ids aren't obfuscated so the service would trivially see which objects are for which submodule which does kinda suck.
I mean, we should anticipate such a migration, as we have a bunch of users on JWT right now, that might opt to choose the new auth mechanism which seems to be the new default over time? At least I don't think we should make it impossible or have them some custom route just to do that? FWIW, we also have MigratableKVStore, why not allow users to build two independent VssStores with different store_id (but both using same auth) and then migrate between them, e.g., for backup purposes.
Right, my question was only how prevalent it would be. Of course the current API supports it just fine (either with a manual header provider or via the VssStoreBuilder), just a question of if we expect it to be universal so much that we should make the API more complicated for new uses.
3 participants
